Skip to content

Conversation

@CTTY
Copy link
Contributor

@CTTY CTTY commented Jul 6, 2023

Change Logs

Add a new GitHub action to test Hudi spark against Java 17. Some known failures would be excluded as they failed due to Hadoop dependencies like MiniDFSCluster

Impact

None

Risk level (write none, low medium or high below)

None

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@CTTY CTTY changed the title [DNM] Skip tests for Java 17 [HUDI-6509] Skip tests for Java 17 Jul 10, 2023
@CTTY CTTY changed the title [HUDI-6509] Skip tests for Java 17 [HUDI-6509] Add GitHub CI for Java 17 Jul 10, 2023
@CTTY CTTY force-pushed the ctty/master-skiptest-j17 branch from cb10175 to 8ea8e3a Compare July 12, 2023 04:41
@CTTY
Copy link
Contributor Author

CTTY commented Jul 12, 2023

@hudi-bot run azure

@yihua yihua self-assigned this Jul 13, 2023
@CTTY CTTY changed the title [HUDI-6509] Add GitHub CI for Java 17 [DNM] [HUDI-6509] Add GitHub CI for Java 17 Jul 15, 2023
@CTTY CTTY changed the title [DNM] [HUDI-6509] Add GitHub CI for Java 17 [HUDI-6509] Add GitHub CI for Java 17 Jul 16, 2023
@CTTY CTTY mentioned this pull request Jul 16, 2023
4 tasks
@CTTY CTTY force-pushed the ctty/master-skiptest-j17 branch from a038fd6 to c79f37c Compare July 16, 2023 06:12
@yihua yihua added dependencies Dependency updates priority:blocker Production down; release blocker release-0.14.0 labels Jul 18, 2023
@CTTY
Copy link
Contributor Author

CTTY commented Jul 18, 2023

It seems validate-bundles(flink1.17, Spark 3.4, Spark 3.4.0) just consistently fail with JDK17 on issue below:

Connecting to jdbc:hive2://localhost:10000/default
23/07/17 17:45:48 [main]: WARN jdbc.HiveConnection: Failed to connect to localhost:10000
Could not open connection to the HS2 server. Please check the server URI and if the URI is correct, then ask the administrator to check the server status.
Error: Could not open client transport with JDBC Uri: jdbc:hive2://localhost:10000/default: java.net.ConnectException: Connection refused (Connection refused) (state=08S01,code=0)
Cannot run commands specified using -e. No current connection
Error: validate.sh HiveQL validation failed.
Error: Process completed with exit code 1.

Need to look into this, otherwise everything looks good. newly added docker-test-java17 and test-spark-java17 are working fine

run:
mvn test -Pfunctional-tests -Pjava17 -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -pl "$SPARK_COMMON_MODULES,$SPARK_MODULES" $MVN_ARGS

docker-test-java17:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be run with validate-bundles since it already validates bundles on Java 17? Any reason to have a separate job here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those tests are generally the same as bundle validation but there is one difference: It requires building Hudi within Docker as it runs tests with mvn test command, while bundle validation build Hudi outside of Docker and only copy jars/bundles to Docker. If we consolidates them as one then the job would need to build twice, it would make the job much slower.

If we just seperate them into 2 jobs then docker test can only build hudi-common modules in Docker which is relatively fast and bundle validation can keep the same behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to have one job and docker setup so it's easier to maintain. We can fix the repeated mvn build later on.

# - <dataset name>/schema.avsc
# - <dataset name>/data/<data files>
#################################################################################################

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we consolidate the test logic of this into validate.sh and reuse existing validate-bundle job?

<flink.connector.kafka.artifactId>flink-connector-kafka</flink.connector.kafka.artifactId>
<flink.hadoop.compatibility.artifactId>flink-hadoop-compatibility_2.12</flink.hadoop.compatibility.artifactId>
<rocksdbjni.version>5.17.2</rocksdbjni.version>
<rocksdbjni.version>7.5.3</rocksdbjni.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does RocksDB 5.17.2 not work? Dependency version upgrade has larger impact.

Copy link
Contributor Author

@CTTY CTTY Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, RocksDB 5.17.2 would throw NoClassDefException when running TestHoodieLogFormat

[ERROR] testBasicAppendAndScanMultipleFiles{DiskMapType, boolean, boolean, boolean}[10]  Time elapsed: 0.118 s  <<< ERROR!
2023-07-13T23:41:36.1420947Z java.lang.NoClassDefFoundError: Could not initialize class org.rocksdb.DBOptions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fishy. Does this only happen for Java 17?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for Java 8 it works fine. This issue only pops up for Java 17

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the code, RocksDbBasedFileSystemView is the only place using RocksDb. So this should be ok.

@CTTY CTTY force-pushed the ctty/master-skiptest-j17 branch from 95b9ca4 to 9bc5072 Compare July 24, 2023 06:29
@hudi-bot
Copy link
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Copy link
Contributor

@yihua yihua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for adding validations on Java 17!

SCALA_PROFILE: ${{ matrix.scalaProfile }}
SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_MODULES: ${{ matrix.sparkModules }}
if: ${{ !endsWith(env.SPARK_PROFILE, '3.2') }} # skip test spark 3.2 as it's covered by Azure CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not required.

SCALA_PROFILE: ${{ matrix.scalaProfile }}
SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_MODULES: ${{ matrix.sparkModules }}
if: ${{ !endsWith(env.SPARK_PROFILE, '3.2') }} # skip test spark 3.2 as it's covered by Azure CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not required.

SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_RUNTIME: ${{ matrix.sparkRuntime }}
SCALA_PROFILE: 'scala-2.12'
if: ${{ env.SPARK_PROFILE >= 'spark3.4' }} # Only support Spark 3.4 for now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not required.

Comment on lines +26 to +65
if [[ ${SPARK_RUNTIME} == 'spark2.4.8' ]]; then
HADOOP_VERSION=2.7.7
HIVE_VERSION=2.3.9
DERBY_VERSION=10.10.2.0
FLINK_VERSION=1.13.6
SPARK_VERSION=2.4.8
SPARK_HADOOP_VERSION=2.7
CONFLUENT_VERSION=5.5.12
KAFKA_CONNECT_HDFS_VERSION=10.1.13
IMAGE_TAG=flink1136hive239spark248
elif [[ ${SPARK_RUNTIME} == 'spark3.0.2' ]]; then
HADOOP_VERSION=2.7.7
HIVE_VERSION=3.1.3
DERBY_VERSION=10.14.1.0
FLINK_VERSION=1.14.6
SPARK_VERSION=3.0.2
SPARK_HADOOP_VERSION=2.7
CONFLUENT_VERSION=5.5.12
KAFKA_CONNECT_HDFS_VERSION=10.1.13
IMAGE_TAG=flink1146hive313spark302
elif [[ ${SPARK_RUNTIME} == 'spark3.1.3' ]]; then
HADOOP_VERSION=2.7.7
HIVE_VERSION=3.1.3
DERBY_VERSION=10.14.1.0
FLINK_VERSION=1.13.6
SPARK_VERSION=3.1.3
SPARK_HADOOP_VERSION=2.7
CONFLUENT_VERSION=5.5.12
KAFKA_CONNECT_HDFS_VERSION=10.1.13
IMAGE_TAG=flink1136hive313spark313
elif [[ ${SPARK_RUNTIME} == 'spark3.2.3' ]]; then
HADOOP_VERSION=2.7.7
HIVE_VERSION=3.1.3
DERBY_VERSION=10.14.1.0
FLINK_VERSION=1.14.6
SPARK_VERSION=3.2.3
SPARK_HADOOP_VERSION=2.7
CONFLUENT_VERSION=5.5.12
KAFKA_CONNECT_HDFS_VERSION=10.1.13
IMAGE_TAG=flink1146hive313spark323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: older Spark versions can be removed.

<flink.connector.kafka.artifactId>flink-connector-kafka</flink.connector.kafka.artifactId>
<flink.hadoop.compatibility.artifactId>flink-hadoop-compatibility_2.12</flink.hadoop.compatibility.artifactId>
<rocksdbjni.version>5.17.2</rocksdbjni.version>
<rocksdbjni.version>7.5.3</rocksdbjni.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the code, RocksDbBasedFileSystemView is the only place using RocksDb. So this should be ok.

@yihua
Copy link
Contributor

yihua commented Jul 25, 2023

CI has passed for 6b33d37. No need to rerun CI again.
Screenshot 2023-07-25 at 12 04 48

@yihua yihua merged commit 03bc554 into apache:master Jul 25, 2023
@CTTY CTTY deleted the ctty/master-skiptest-j17 branch July 25, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates priority:blocker Production down; release blocker release-0.14.0

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants